Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Changed altivec.rs to new intrinsic declaration #1722

Merged
merged 1 commit into from
Feb 25, 2025
Merged

Changed altivec.rs to new intrinsic declaration #1722

merged 1 commit into from
Feb 25, 2025
+2 −15

Conversation

BLANKatGITHUB
Copy link
Contributor

No description provided.

@rustbot
Copy link
Collaborator

rustbot commented Feb 23, 2025

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@eduardosm
Copy link
Contributor

It think you can call crate::intrinsics::copy_nonoverlapping directly to avoid declaring the intrinsic.

@BLANKatGITHUB
Copy link
Contributor Author

Well in rust-lang/rust#132735 the pr said to migrate it to the same style like in compiler and standard library , I will change it if its not ok

@eduardosm
Copy link
Contributor

The goal is to avoid having an extern "rust-intrinsic" block, which can also be achieved by not needing to declare the intrinsic.

@folkertdev
Copy link
Contributor

The code does note that the intrinsic declaration is there to

// Workaround ptr::copy_nonoverlapping not being inlined

there are some issues around target features preventing inlining, so it may be related to that. That problem also may have been fixed in the meantime (by more aggressively inlining on the rust side), but it's something to check for.

@folkertdev
Copy link
Contributor

Looks like that is no longer a problem (in this case) https://godbolt.org/z/vYzxKMdeo, and both versions generate the same assembly. So just using core::ptr::copy_nonoverlapping should be fine. (I don't even see a reason to prefer the intrinsic from core::intrinsics, but maybe there is one?)

RalfJung
RalfJung reviewed Feb 24, 2025
View reviewed changes
crates/core_arch/src/powerpc/altivec.rs Outdated
@@ -688,10 +688,13 @@ mod sealed {
let addr = (b as *const u8).offset(a);

// Workaround ptr::copy_nonoverlapping not being inlined
unsafe extern "rust-intrinsic" {
#[rustc_intrinsic]
#[rustc_intrinsic_must_be_overridden]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the rustc_intrinsic_must_be_overridden attribute is being removed, please do not use it any more. The new new way to declare an intrinsic is:

#[rustc_intrinsic]
unsafe fn unreachable() -> !;

@RalfJung
Copy link
Member

RalfJung commented Feb 24, 2025 •
edited
Loading

We used to do something similar in core::ptr where we sometimes called the intrinsic directly to avoid the extra debug checks that exist in core::intrinsics::copy_nonoverlapping. But we no longer seem to do that so I agree we shouldn't have to do it here either -- just call core::ptr::copy_nonoverlapping.

@RalfJung
Copy link
Member

You can then also remove this line; stdarch no longer should declare any intrinsics itself:

https://github.com/rust-lang/stdarch//blob/f32eeffb47d783a703017851b1cf6bdecf1601c0/crates/core_arch/src/lib.rs#L16

@BLANKatGITHUB
Copy link
Contributor Author

You can then also remove this line; stdarch no longer should declare any intrinsics itself:

https://github.com/rust-lang/stdarch//blob/f32eeffb47d783a703017851b1cf6bdecf1601c0/crates/core_arch/src/lib.rs#L16

wont it potentially break some of the functionality?

@folkertdev
Copy link
Contributor

No, you've removed all usages of rust-intrinsic, so this feature is no longer needed (and in fact we'd want adding a new rustc-intrinsic to fail). Also CI would catch it if something did break, so there is no risk.

RalfJung
RalfJung reviewed Feb 25, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This crate has a sumewhat funny import structure due to how it is integrated with libcore. Everything else uses crate::ptr, not core::ptr, so just to be sure let's also use that here.

crates/core_arch/src/powerpc/altivec.rs Outdated
let mut r = mem::MaybeUninit::uninit();

copy_nonoverlapping(
core::ptr::copy_nonoverlapping(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
core::ptr::copy_nonoverlapping(
crate::ptr::copy_nonoverlapping(

crates/core_arch/src/powerpc/altivec.rs Outdated
}

copy_nonoverlapping(
core::ptr::copy_nonoverlapping(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
core::ptr::copy_nonoverlapping(
crate::ptr::copy_nonoverlapping(

@BLANKatGITHUB
replaced extern "rust-intrinsic" block with core::ptr::copy_nonoverla…
b12757c 
…pping

using core::ptr::copy_nonoverlapping for memory operations

changes core::ptr::coopy_nonoverlapping to crate::ptr::
RalfJung
RalfJung approved these changes Feb 25, 2025
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!
(But I can't land changes in this repo.)

@Amanieu Amanieu added this pull request to the merge queue Feb 25, 2025
Merged via the queue into rust-lang:master with commit 1d71dc6 Feb 25, 2025
60 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@RalfJung RalfJung RalfJung approved these changes

Assignees

@Amanieu Amanieu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BLANKatGITHUB @rustbot @eduardosm @folkertdev @RalfJung @Amanieu